Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make load_profile importable from aiida module #6609

Merged
merged 5 commits into from
Dec 2, 2024
Merged

Make load_profile importable from aiida module #6609

merged 5 commits into from
Dec 2, 2024

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Nov 13, 2024

fixes #6608

load_profile() is widely used and documented. Exposing it as API under aiida module directly can make a bunch of LSP happy.

@unkcpz unkcpz requested a review from GeigerJ2 November 13, 2024 22:55
@codecov Codecov
Copy link

codecov bot commented Nov 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.89%. Comparing base (ef60b66) to head (8bb411f).
Report is 153 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/__init__.py 60.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6609      +/-   ##
==========================================
+ Coverage   77.51%   77.89%   +0.39%     
==========================================
  Files         560      567       +7     
  Lines       41444    42181     +737     
==========================================
+ Hits        32120    32854     +734     
- Misses       9324     9327       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz unkcpz force-pushed the enh/load-profile-api branch 2 times, most recently from 59d7923 to bb35d26 Compare November 14, 2024 02:28
unkcpz
unkcpz commented Nov 14, 2024
View reviewed changes
src/aiida/__init__.py Outdated Show resolved Hide resolved
@danielhollas danielhollas self-requested a review November 14, 2024 12:07
@danielhollas
Copy link
Collaborator

danielhollas commented Nov 14, 2024 •
edited
Loading

Exposing it as API under aiida module directly can make a bunch of LSP happy.

Out of curiosity, which LSPs specifically are you talking about?

The load_profile is exposed from aiida package. The __all__ attribute influences only the star imports (from aiida import *). So I don't understand why an LSP should complain.

https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

@unkcpz
Copy link
Member Author

unkcpz commented Nov 14, 2024 •
edited
Loading

Out of curiosity, which LSPs specifically are you talking about?

The load_profile is exposed from aiida package. The all attribute influences only the star imports (from aiida import *). So I don't understand why an LSP should complain.

I use basedpyright, and I think it is also good to have a list of what APIs we are going to exposed explicitly in all, besides make LSP happy.

@unkcpz unkcpz marked this pull request as draft November 14, 2024 16:56
@unkcpz unkcpz mentioned this pull request Nov 15, 2024
Merged
2 tasks
@danielhollas danielhollas mentioned this pull request Nov 15, 2024
Closed
@unkcpz unkcpz force-pushed the enh/load-profile-api branch from ec71c9f to d4c4954 Compare December 2, 2024 10:21
@unkcpz unkcpz marked this pull request as ready for review December 2, 2024 10:23
@unkcpz unkcpz requested a review from danielhollas December 2, 2024 10:26
@unkcpz
Copy link
Member Author

unkcpz commented Dec 2, 2024 •
edited
Loading

Hi @danielhollas, I update this PR. It is ready to review.

The plan for the APIs was discussed during the coding week and it came with the PR #6636. For more general discussion about APIs we can go there.

danielhollas
danielhollas approved these changes Dec 2, 2024
View reviewed changes
Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite confused by the ipython loader change, so if you can extend the docstring that would be helpful. Otherwise LGTM, thanks!

src/aiida/__init__.py Outdated Show resolved Hide resolved
danielhollas
danielhollas reviewed Dec 2, 2024
View reviewed changes
src/aiida/__init__.py Outdated
from .tools.ipython.ipython_magics import AiiDALoaderMagics
"""Load the AiiDA IPython extension, using ``%load_ext aiida``.

:param ipython: InteractiveShell instance. If omitted, the global InteractiveShell is used.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

In this function, the ipython argument is not optional (it doesn't have a default value). So I think the docstring needs to be reworded

Suggested change
:param ipython: InteractiveShell instance. If omitted, the global InteractiveShell is used.
:param ipython: InteractiveShell instance. If None, the global InteractiveShell is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks right, thanks for careful reviewing.

@unkcpz unkcpz force-pushed the enh/load-profile-api branch from 5107779 to 8bb411f Compare December 2, 2024 21:24
@unkcpz unkcpz merged commit ec52f4e into aiidateam:main Dec 2, 2024
7 of 8 checks passed
@unkcpz unkcpz deleted the enh/load-profile-api branch December 2, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhollas danielhollas danielhollas approved these changes

@GeigerJ2 GeigerJ2 Awaiting requested review from GeigerJ2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"aiida" module does not have the __all__ attribute
2 participants
@unkcpz @danielhollas